fix: build errors canceling deploy contexts #1225
Conversation
wesbillman
requested review from
alecthomas,
dhanji,
worstell and
matt2e
as code owners
wesbillman
force-pushed
the
fix-build-engine-context
branch
from
5955849
to
79b3a08
Compare
|
@alecthomas maybe we can chat about this approach and maybe if there's a better way to clean this up. The summary of what I want this change to do it to try to build and deploy whatever it can and then return the errors after it's attempted everything. The reason for that is sort of described above, but maybe not super clearly. During our build deploy loops we would cancel the whole loop if an error occurs. If this happens during initial deploy we might not build or deploy modules because of some previous error. So now with this change we attempt all of them (assuming their dependencies have build). This cleaned up the code in some places, but made it perhaps more confusing in other areas. Open to refactor ideas as well if there's something we can do to make this more obvious. |
|
I also want to validate that we want to attempt to build and deploy as much as possible vs. erroring early when a build or deploy error occurs. If we error early, we might have to keep a record of what was skipped because of the error so we can build them when the error is fixed. Right now, we fix the error and the things that haven't built will not build until a change is made to that module. |
buildengine/engine.go
Outdated
| if module, ok := project.(Module); ok { | ||
| err := Deploy(deployCtx, module, replicas, waitForDeployOnline, e.client) | ||
| if err != nil { | ||
| deployErrors = append(deployErrors, err) |
There was a problem hiding this comment.
It's not safe to concurrently append to a slice.
buildengine/engine.go
Outdated
| deployQueue := make(chan Project, len(projects)) | ||
| wg, ctx := errgroup.WithContext(ctx) | ||
| buildGroup, buildCtx := errgroup.WithContext(ctx) | ||
| deployGroup, deployCtx := errgroup.WithContext(ctx) |
There was a problem hiding this comment.
If your goal is to avoid cancelling the context on failure, don't use errgroup.WithContext() - instead create an errgroup manually like so wg := errgroup.Group{}
wesbillman
force-pushed
the
fix-build-engine-context
branch
from
79b3a08
to
c223f4f
Compare
wesbillman
force-pushed
the
fix-build-engine-context
branch
from
c223f4f
to
0ef1803
Compare
|
@alecthomas I merged this one after some updates to try to get updates to the other teams. But happy to make more changes if I missed anything here. |
Fixes an issue where build errors would cancel the deploys for modules that build successfully.
For example, if we have
timeandecho(echo depends on time).Then, we fix the build error in
echo, but time never deployed and since it doesn't depend onechoit's never triggered to rebuild (unless a change to time is made).This change introduces a
buildCtxanddeployCtxIf there are any build failures we will report them after the deploys for successful builds complete.